Conversation
abachma2
left a comment
There was a problem hiding this comment.
This generally looks good to me. I recommend some of the capitalization is consistent with 1856 in Cyclus.
| cycstub --type facility :stublibrary:StubFacility | ||
| cycstub --type institution :stublibrary:StubInstitution | ||
| cycstub --type region :stublibrary:StubRegion |
There was a problem hiding this comment.
Do we want these to have CamelCase in the library name?
There was a problem hiding this comment.
I did it this way before I committed to fixing the CamelCase problem. In the fixed version, all combinations will work successfully, so these don't need to change.
source/arche/hello_world_cpp.rst
Outdated
| This pages walks you through a very simple hello world example using | ||
| |cyclus| agents. First make sure that you have the dependencies installed, | ||
| namely |Cyclus|, CMake, and a recent version of Python (2.7 or 3.3+). | ||
| namely |Cyclus|, CMake, and a recent version of Python (3.3+). |
There was a problem hiding this comment.
I'm not sure what our minimum version is these days????
There was a problem hiding this comment.
My guess would be python 3.9, but I can look around.
There was a problem hiding this comment.
Going off of the dependencies for installing cyclus via conda, it looks like 3.8 is the lowest version allowed. I can build cyclus with 3.8 locally, but the unit tests won't pass unless you have python 3.11+, as of commit 4d1eeef5
|
I reverted this to draft because a little more work needs to be done to make it all consistent and useful. |
|
I think this is probably where this needs to be to be consistent now. |
dean-krueger
left a comment
There was a problem hiding this comment.
The only two comments I have are the same PascalCase vs camelCase comment I made in the other PR #1856, and that (and I'm not sure if this really needs to be fixed), but a naive version of me would probably try to type the command tutorial $ cycstub --type inst tutorial:TutorialLibrary:TutorialInstitution instead of the command cycstub --type inst tutorial:TutorialLibrary:TutorialInstitution in the tutorial folder. Perhaps if you copy the command from the markdown file it is smart enough to not give you that part, but just a thought! Otherwise this looks good to me.
I think we've often wondered whether it makes sense to include the prompt in the samples. I'm torn on this. The only real benefit is that you can see the directory change and the user gets some clear feedback on which directory they should be in to run the command. The downside is that it looks like part of the command... we could try a different prompt separator... maybe: |
|
I'm not sure the separator is really the problem, since ultimately this "issue" is with people who aren't that familiar with using the terminal. I wonder how other people handle this when writing documentation. I guess my personal preference is to change: """ """ Into something like: """ From the "tutorial" directory, run: """ But perhaps that's too verbose/much overhead/easy to miss? Ultimately I don't think it matters much (except that actually this makes the commands really easy to copy form the markdown file and paste in your terminal since you don't have to remove the |
|
I just updated all the |
A few typos and copyedit improvements Co-authored-by: Amanda Bachmann <abachmann@anl.gov>
5f0bae3 to
6ad43cf
Compare
|
All the bash code blocks now have sytnax highlighting that requires the prompt to be indicated with only a single |
dean-krueger
left a comment
There was a problem hiding this comment.
Thanks @gonuke for making that change. This looks good to me now. I'll leave it unmerged until Wednesday morning to give others a chance to look at it on Monday if they want (I know @abachma2 mentioned wanting some time after TWOFCS to check things). If others get around to it before then, and approve, feel free to merge.
source/arche/hello_world_cpp.rst
Outdated
| This pages walks you through a very simple hello world example using | ||
| |cyclus| agents. First make sure that you have the dependencies installed, | ||
| namely |Cyclus|, CMake, and a recent version of Python (2.7 or 3.3+). | ||
| namely |Cyclus|, CMake, and a recent version of Python (3.3+). |
There was a problem hiding this comment.
Going off of the dependencies for installing cyclus via conda, it looks like 3.8 is the lowest version allowed. I can build cyclus with 3.8 locally, but the unit tests won't pass unless you have python 3.11+, as of commit 4d1eeef5
This removes references to the cycstub repo and instead replaces it with reference to the cycstub utility. This leaves one file untouched because it is slated to be removed in #418 .
cmake.rststill needs a closer review, but this could be left to a different PR??Fixes #197 and #366